Add promise return for getSocket #179
Conversation
MasterOdin
force-pushed
the
feat-socket-promise
branch
2 times, most recently
from
7e77b11 to
a15c15a
Compare
| portfinder.getSocket({ | ||
| path: path.join(badDir, 'test.sock'), | ||
| }, function () { | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
Calling getSocket here was probably not meaningfully necessary to waiting for createServers to finish, so I removed it in favor of just calling done.
test/port-finder-socket.test.js
Outdated
| if (process.platform === 'win32') { | ||
| fs.unlinkSync(sock); | ||
| } |
There was a problem hiding this comment.
The stopServers function meaning shouldn't have changed with the move to async.each beyond just being more concise, though this block is new, and necessary for cleaning up sockets on windows due to the above symlink shenanigans.
| }); | ||
| } else { | ||
| return internals.getPorts(count, options, callback); | ||
| internals.getPorts(count, options, callback); |
There was a problem hiding this comment.
This and the change to internals.getPort above are no-ops in terms of what they do (as the functions don't return anything), but I feel this is more "semantically correct" in that we don't expect this function to return anything if not a promise.
MasterOdin
force-pushed
the
feat-socket-promise
branch
2 times, most recently
from
5ec34aa to
ecffd0c
Compare
Signed-off-by: Matthew Peveler <[email protected]>
MasterOdin
force-pushed
the
feat-socket-promise
branch
from
ecffd0c to
f4e9d55
Compare
| // We don't use `test.sock` here due to some race condition on Windows in freeing the `test.sock` file | ||
| // when we close the servers. | ||
| test('with a directory that exists should respond with the first free socket (foo.sock)', function (done) { |
There was a problem hiding this comment.
The test was mentioning test.sock, but the code was using exists.sock. I'm guessing this was due to an issue on windows where the socket files were not being cleared as part of cleanup. While this was fixed, I think there's still some weird race condition where calling this function too quickly will still fail on Windows if using test.sock.
As such, I went with calling it foo.sock to avoid the name collision and that I renamed it as I didn't like calling it exists.sock as that felt like it was implying the socket file already existed.
| name = base === 0 ? 'test.sock' : 'test' + base + '.sock'; | ||
| let sock = path.join(socketDir, name); | ||
| const socket = path.join(socketDir, name); | ||
| let sock = socket; |
There was a problem hiding this comment.
Can we just have 1 variable socket? Or am I missing something?
There was a problem hiding this comment.
If the platform is windows, we append the pipe stuff to the front of sock to be passed to server.listen. However, we create the file without the pipe stuff, so that's why I've used the two separate variables, socket to track the filename as generated, and then sock to be modified with the pipe stuff.
|
I cleaned up my comments. I see windows is a little feisty :) Just one question about variable naming. The rest all makes sense. This is awesome work, thank you very much, I really appreciate it 👍 |
|
Thanks again @MasterOdin |
2 participants
Closes #169
Adds a promise return for
getSocketas well asgetSocketPromisefunction, so as to match thegetPortandgetPortsfunctions. Also adds type information for these functions as that was totally missing before.The overall tests that are run remain the same, just that they're run against all three variants as was setup in #172.